-
Notifications
You must be signed in to change notification settings - Fork 377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restore full functionality to trim_headers (untrim) #1270
Restore full functionality to trim_headers (untrim) #1270
Conversation
@@ -5,6 +5,11 @@ | |||
|
|||
#include <chain.h> | |||
#include <util/time.h> | |||
#include <validation.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a circular dependency.
"chain.h -> validation.h -> chain.h"
I'm not quite sure how to fix it -- we can't simply forward declare ChainManager
here since the line below requires knowledge of the fields of ChainManager
:
m_pcontext->chainman->m_blockman.m_dirty_blockindex.insert(this);
@@ -1710,7 +1711,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) | |||
|
|||
// if pruning, unset the service bit and perform the initial blockstore prune | |||
// after any wallet rescanning has taken place. | |||
if (fPruneMode || node::fTrimHeaders) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was || node::fTrimHeaders
removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conditional is unsetting NODE_NETWORK, without this flag enabled it means this peer can't serve historical block data (ie. it's a pruned node). This was previously necessary because we weren't able to "untrim" the headers, which this PR fixes.
…ntrimmed on demand now
…calling SetNodeContext
4b47709
to
b4185c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
140b559
to
d67bc6f
Compare
(cherry picked from commit 630870f)
d67bc6f
to
4171939
Compare
d67aecb
to
eed85dc
Compare
I have been running a pruned Elements node with this patch continuously for months and have seen no issues at all. That said, memory usage is still unacceptably high at 2.3 GiB. Keeping the entire chain of block header hashes, going back to the genesis block, in RAM at all times is unsustainable, especially since a new block is added to the Liquid chain every minute. It means |
UTACK eed85dc on all the additions. |
Hey, yeap, I agree that we need to continue reducing the usage, but this is the most we can do without diverging from what bitcoind does, any change after that would have to be much more invasive |
Currently, trim_headers has certain limitations wrt the way it behaves in P2P protocol and in certain RPC calls. This PR brings back the full functionality and fixes a corruption issue when combined with pruning.
Fixes: #1241